Fix validation errors for struct.wait#8394
Conversation
tlively
left a comment
tlively
left a comment
There was a problem hiding this comment.
It would be best to match how we validate field indices for struct.get and struct.set, etc. I notice that's not done in IRBuilder.
I took a look and I think the struct.set validation isn't correct today. For the null/unreachable case we drop the type immediate and infer a type that's none or unreachable, in which case some of the relevant checks are skipped (link). I checked with this module which should fail validation but passes (the arg doesn't match the struct definition and the struct field isn't mutable): (module
(type $t (struct (field i32)))
(func
(struct.set $t 0 (ref.null $t) (f32.const 0))
)
)
I think IRBuilder is the right place for these validations because we still have the type immediate there, does that sound correct to you? I can also look at enabling the struct.wast spec test which is disabled now and hopefully covers this case. |
tlively
left a comment
tlively
left a comment
There was a problem hiding this comment.
Makes sense, thanks. I guess we should make the same change for all other operations that take field index immediates.
2 participants
Prior to #8378, any
assert_invalidassertions that required a feature to be enabled spuriously succeeded even if the test would otherwise wrongly pass validation. The checks that were in wasm-validator.cpp didn't run if the ref was null or unreachable, which is wrong because the type + field index immediates could still be wrong.Move these assertions to IRBuilder where we have the type immediate available.
Verified by removing the assert_invalid parts and checking that the error message matches what's in the test.
Part of #8315.